Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make static methods that ought to be static. Check if was intialized otherwise #35

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

facontidavide
Copy link

Hi,

thanks for this nice utility.

I noticed that there are tow function that can be used outside the class GeodeticConverter.
I think it is more flexible to keep them static members or, even better, stand alone functions.

I also added exceptions when the other methods are used without initialization.

By personal opinion is that initialization is mandatory and that the constructor should force that, but I don't want to break the code of libraries which depend on this one.

Cheers

Davide

@marija-p
Copy link

@rikba Could you have a look?

@marija-p
Copy link

Please 😄

Copy link
Contributor

@rikba rikba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution and exposing the functionality and throwing initialization errors. I think that's very valuable. SCOLGTM.


inline double rad2Deg(const double radians)
{
return (radians / M_PI) * 180.0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make this a constant, i.e.,
const double kRad2Deg = 180.0 / M_PI;

Saves some devisions.


inline double deg2Rad(const double degrees)
{
return (degrees / 180.0) * M_PI;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

include/geodetic_utils/geodetic_conv.hpp Show resolved Hide resolved
include/geodetic_utils/geodetic_conv.hpp Show resolved Hide resolved
include/geodetic_utils/geodetic_conv.hpp Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants